Resolve hosts declared with multiple aliases in ssh config (fixes #12) - #13
Conversation
`Host docker-lxc hlab` was unreachable under *either* alias.
ssh-config@5 returns a plain string for a single-token value but an array
of token objects ({val, separator, quoted}) once a multi-value directive
carries more than one token. extractHostsFromConfig stored that array in
`alias` verbatim, so every strict comparison downstream
(_assertKnownHostAlias, getHostInfo, getPasswordForHost, getAllKnownHosts)
compared a string against an array and never matched. The host was listed
by listKnownHosts but rejected by the whitelist gate before ssh was ever
spawned - even though native ssh resolves such aliases fine.
Normalize ssh-config values once at parse time: keep `alias` as the first
alias (output shape unchanged) and add `aliases` with the full list, then
match through a hostMatchesAlias helper.
Also fixed by the same normalization:
- `Host * !bastion` was not skipped, because a multi-token Host value is
an array and never strictly equals '*'
- multi-token directives such as ProxyCommand were surfaced as arrays of
token objects in listKnownHosts output instead of readable strings
Verified against a real 29-block ssh config: 40 aliases now resolve, 0
raw arrays remain, host count in listKnownHosts output is unchanged.
14 tests silently asserted POSIX-only behaviour and failed when the suite ran on Windows: the chmod 600 config check (a no-op there), the /bin/sh askpass helper (a .cmd batch file there), the `detached` spawn flag (POSIX only) and a bare 'ssh' as argv[0] (an absolute ssh.exe path there). Rather than skipping them on Windows, both platform paths are now asserted explicitly: loadServerAs() re-imports server.mjs with process.platform faked, so the Windows branches are covered from any host OS. SSH_BIN and SCP_BIN are exported so tests assert against the binary the module actually resolved instead of hardcoding a name. Coverage of server.mjs is now 100% of statements, branches, functions and lines, with those thresholds pinned in vitest.config.mjs so a change adding an untested line fails the build. New tests cover the Windows PATH/PATHEXT walk in resolveExecutable, the askpass exit/SIGINT/SIGTERM handlers, silent mode, hostAlias type validation, the known_hosts matching fallback, glob expansion of Include directives, output-truncation markers and the tool dispatch timeout default. Also drops the `|| process.env.Path` fallback in resolveExecutable: Node exposes process.env case-insensitively on Windows, so process.env.PATH already resolves a variable spelled `Path` and the fallback was unreachable (and therefore uncoverable on the platform it was written for). CI now runs the matrix on windows-latest as well as ubuntu-latest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Windows checks out CRLF by default (core.autocrlf=true). server.mjs opens with a `#!/usr/bin/env node` shebang, and the Vite SSR transform vitest runs the module through cannot parse that shebang with CRLF line endings: it hoists the imports above the shebang and then fails on "Invalid Character `!`". The whole suite aborts with a SyntaxError before a single test runs. This predates the multi-alias fix — a CRLF checkout of a5f9ca3 fails the same way. It only surfaced now because CI never ran on Windows before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MCP Server Handlers block drives the real SSHClient that main() constructs, so every tool call spawned an actual ssh/scp process against 1.2.3.4 and blocked on the network. 'should handle runCommandBatch tool call' issues two such commands and exceeded the 5s test timeout on Windows CI runners, where the suite runs ~30x slower than locally. Stub the three process-starting methods for that block: it asserts tool dispatch and response shape, which is unaffected, and the methods themselves are covered by the SSHClient tests. Also raises testTimeout to 15s for headroom on slow runners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
12 high, 4 moderate, 1 low had accumulated since the 1.3.7 cleanup, and the CI audit step blocks on high. npm audit fix is unusable here — it aborts with an internal npm error (Cannot read properties of null reading 'edgesOut') on this tree's overrides — so the fixes are pinned explicitly. Direct bumps, all inside the existing semver ranges: @modelcontextprotocol/sdk 1.27.1 -> 1.30.0, vitest and @vitest/coverage-v8 4.1.4 -> 4.1.10, @anthropic-ai/dxt 0.2.5 -> 0.2.6. That cleared vite, postcss and nanoid. Raised the existing tmp override to >=0.2.6, clearing the whole @anthropic-ai/dxt -> @inquirer/prompts -> @inquirer/editor -> external-editor -> tmp chain. Added overrides for the transitive HTTP-stack advisories reachable through the MCP SDK: brace-expansion, fast-uri, ip-address, hono, @hono/node-server, body-parser, qs, express-rate-limit — each pinned to the lowest version carrying the fix. As in 1.3.7 none of that code is reachable from this package: it belongs to the SDK's HTTP/SSE transport and mcp-ssh only loads server/stdio.js. npm audit now reports zero vulnerabilities. Verified the suite stays green at 100% coverage, npm ci reproduces from the lockfile, and the real server still answers initialize and tools/list over STDIO. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merged — thanks for this. The root-cause analysis was spot on: I added four commits on top before merging. Summary of what changed and why:
You flagged that 16 tests fail on Windows on
Rather than skipping them on Windows — which would leave coverage holes exactly where the platform-specific code lives — both paths are now asserted explicitly. A Coverage of
This one predates your PR. Windows checks out CRLF by default (
Also pre-existing. The
Unrelated to your change, but it was blocking the CI audit step. 12 high / 4 moderate / 1 low had accumulated since the 1.3.7 cleanup. CI now runs Thanks again for the fix and for the thorough write-up in the PR description — it made this easy to review. |
2 participants
Fixes #12.
Host docker-lxc hlabwas unreachable under either alias —_assertKnownHostAliasrejected the request beforesshwas ever spawned, even though nativesshresolves such aliases fine.Cause
ssh-config@5returns a plain string for a single-token value but an array of token objects ({val, separator, quoted}) once a multi-value directive carries more than one token.extractHostsFromConfigstored that array inaliasverbatim, so every strict comparison downstream compared a string against an array and never matched.Change
Normalize ssh-config values once, at parse time:
aliaskeeps holding the first alias, so the shape oflistKnownHostsoutput is unchanged for existing single-alias configsaliasesfield carries the full listhostMatchesAliashelper, replacing four hand-rolled===chains (_assertKnownHostAlias,getHostInfo,getPasswordForHost,getAllKnownHosts)Two related defects fall out of the same normalization:
Host * !bastionis now skipped. The oldsection.value !== '*'check could not match a multi-token value, so such a block was emitted as a host if it carried aHostName.ProxyCommand,SendEnv, …) are flattened to strings instead of being surfaced as arrays of token objects.Tests
Four regression tests in
server.test.mjs, all underSSHConfigParser > extractHostsFromConfig:aliasstays a string for single-alias hosts (guards the output shape)Verified they fail on the unpatched parser and pass with the fix.
Verification
Against a real 29-block ssh config: 40 aliases resolve, 0 raw arrays remain, host count in
listKnownHostsoutput unchanged (29 → 29).Note on the suite: on Windows 16 of the pre-existing tests fail on
mainbefore this change (POSIXchmodpermission checks, the askpass shell script, andspawn/scpmocks). That set is byte-identical before and after this patch — no regressions introduced, but the suite is not green on Windows to begin with.